Skip to content

QDB-16178 - Refactor statistics#90

Closed
alivasples wants to merge 3 commits intomasterfrom
sc-16178/refactor-python-statistics-module-to-make-use
Closed

QDB-16178 - Refactor statistics#90
alivasples wants to merge 3 commits intomasterfrom
sc-16178/refactor-python-statistics-module-to-make-use

Conversation

@alivasples
Copy link

No description provided.

@solatis solatis changed the title refactoring statistics QDB-16178 - Refactor statistics Apr 28, 2025
@qodo-code-review
Copy link

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Incorrect Logging

The _calculate_delta_stat function logs the previous value twice instead of showing previous and current values. This will make debugging difficult.

logger.info(
    "calculating delta for stat_id = {}, prev = {}. cur = {}".format(
        stat_id, str(prev_stat["value"]), str(prev_stat["value"])
    )
Deprecated Method

The stat_type function is marked as deprecated with an exception but is still called by other parts of the code or external dependencies. This will cause runtime errors.

def stat_type(stat_id):
    """
    Returns the statistic type by a stat id. Returns one of:

     - 'gauge'
     - 'counter'
     - None in case of unrecognized statistics

    This is useful for determining which value should be reported in a dashboard.
    """
    raise Exception("Deprecated Method.")
Potential Infinite Loop

The _get_all_keys function might enter an infinite loop if the number of keys is exactly equal to n in each iteration, as the condition checks for >= n.

while xs is None or len(xs) >= n:
    if xs is not None:
        n = n * increase_rate
    if n >= MAX_KEYS:
        raise Exception(f"ERROR: Cannot fetch more than {MAX_KEYS} keys.")
    xs = dconn.prefix_get(stats_prefix, n)

@solatis solatis closed this May 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants